Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(inflight): reduce shrinking limit #306

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented May 10, 2023

Part of #298

Inflights are initialized per partition, so limit of 1000000 makes no sense

inflights are initialized per partition, so limit of 1000000 makes no sense
@dkropachev dkropachev added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels May 10, 2023
@@ -21,7 +21,7 @@ import (
// We track inflights in the map, maps in golang are not shrinking
// Therefore we track how many inflights were deleted and when it reaches the limit
// we forcefully recreate the map to shrink it
const shrinkInflightsLimit = 1000000
const shrinkInflightsLimit = 1000
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 1000?
Do we know the cost of map recreation?

Copy link
Collaborator Author

@dkropachev dkropachev May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just guesstimate.

This is limit of deletions to trigger recreation, recreation it self depends on the number of items in the map, which is limited only by number partition size, which is Uint64.Max/partitionCount usually it is 18446744073709551615/10000 = 1844674407370955.
With current implementation this limit is pretty much reachable, which is source of memory consumption.
But after #309 and #307 are fixed we should not see more than pkBufferReuseSize + concurrency, which is usually 100+10 infligths per partition.

Copy link
Collaborator

@vponomaryov vponomaryov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dkropachev dkropachev merged commit 1c69932 into master May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants